-
Notifications
You must be signed in to change notification settings - Fork 6.8k
[WIP] Revert "Remove cleanup on side threads (#19378)" #19432
Conversation
This reverts commit 43750c8.
Hey @barry-jin , Thanks for submitting the PR
CI supported jobs: [centos-gpu, sanity, unix-gpu, website, clang, windows-cpu, edge, centos-cpu, miscellaneous, windows-gpu, unix-cpu] Note: |
I fail to understand how is this a bugfix. The original commit was introduced as a workaround for #19360 and the issue was raised (#19379) that a better solution should be put in its place. Just reverting it is not such better solution. Could you provide details on how this commit triggers the issue you are seeing? |
Sorry for losing the context of PR #19378 . It's definitely not the best solution. But our current issue #19420 has blocked GluonNLP CI and GluonNLP development. So, this PR is an intermediate solution rather than a bugfix to have Cuda 11 broken on CD and unblock GluonNLP development. I will investigate more to find a better solution. |
@mxnet-bot run ci [windows-gpu] |
Jenkins CI successfully triggered : [windows-gpu] |
@sxjscience Sure, I understand that there is an issue somewhere and I'm happy to help resolve it. That said, neither this PR nor the linked issue explains why the failure happens with that change enabled (I assume here that you verified that a commit just before #19378 passes). If this was about reverting a feature it would be fine, but here we are reverting a bugfix (or bug workaround to be more precise), so I think it makes sense to make sure we understand what is going on here before we reintroduce a problem. |
@ptrendx Yeah, I agree with that. I think we will need some time to understand what's happening underneath. |
@ptrendx Thanks. I will update the linked issue with more detailed information after my investigation. |
Description
An intermediate solution to #19420 to unblock GluonNLP CI.
This reverts commit 43750c8.
Checklist
Essentials
Changes
Comments